Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Imagick adapter, handles color profiles better #1750

Draft
wants to merge 23 commits into
base: main
Choose a base branch
from

Conversation

woutersamaey
Copy link
Contributor

Description (*)

Proposal PR for an Imagick adapter.
This one has better image quality and can handle embedded color profiles.
I had a store where the image colors changed because GD could not properly process the ICC profile.
This adapter is better.

I'm just contributing my code for this 1 file and don't want to make a big deal out of it. If someone wants to help complete and integrate further, feel free.

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All automated tests passed successfully (all builds are green)

@github-actions github-actions bot added Component: lib/Varien Relates to lib/Varien Component: lib/* Relates to lib/* labels Jul 30, 2021
@tmotyl
Copy link
Contributor

tmotyl commented Jul 30, 2021

Nice idea!
Please also provide which IM version you have tested it with (different IM versions have different quircks). Maybe a check in the code would also be useful?
Do I get it right that to use this adapter, some additional changes/configuration is required? Can you describe it?

@woutersamaey
Copy link
Contributor Author

@tmotyl unfortunately I don't have the time, but you can look at Varien_Image_Adapter::factory(). This needs to change + some core files + maybe create a config setting somewhere.

I'm not aware of any Imagick quirks to be honest.

@woutersamaey
Copy link
Contributor Author

This adapter now also supports "exif:Orientation" which can cause an image to appear rotated in Magento when it is not rotated on your computer.

@fballiano
Copy link
Contributor

I always use https://github.com/colinmollenhour/Perfect_Watermarks cause the GD adapter never works for me

@fballiano
Copy link
Contributor

I've added the PerfectWatermarks module to https://openmage.readthedocs.io/en/latest/modules/more.html#images-optimization

@fballiano
Copy link
Contributor

@colinmollenhour what do you think about this PR? Asking you cause you're the author of the PerfectWatermarks extension that I normally use in my projects.

@colinmollenhour
Copy link
Member

@fballiano At a quick glance it looks like a huge improvement over the stock code and based on the commit log and Wouter's comments it sounds like it has been through a lot of production usage and tweaking so it is probably a good thing to merge as-is but I just don't have time to test it..

@woutersamaey can you provide some background on all of the testing this has been through and your confidence level that this would not cause any regressions for any user's of the stock Imagemagic adapter (which I think everyone agrees is pretty bad)?

Is anyone else in addition to Wouter using this in production? Just checking to see how many data points there are.

@woutersamaey
Copy link
Contributor Author

I have 1 huge volume site using this in production. They have a lot of custom code, but that should not affect the image handling. I'm very confident is it stable.

@woutersamaey
Copy link
Contributor Author

I just noticed that my class is still called Storefront_MediaStorage_Model_Image_Adapter_Imagick. Guess I need to change this to Varien_Image_Adapter_Imagick? It's been a while...

@colinmollenhour
Copy link
Member

@woutersamaey Yes the class name will need to match the pattern in the file path. :)

fballiano
fballiano previously approved these changes Aug 3, 2022
Copy link
Member

@colinmollenhour colinmollenhour left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems that either the old class Varien_Image_Adapter_Imagemagic should be deleted or a new constant added for it such as ADAPTER_IM_LEGACY.

@fballiano fballiano changed the base branch from 20.0 to main April 4, 2023 17:32
@fballiano
Copy link
Contributor

@colinmollenhour Varien_Image_Adapter_Imagemagic doesn't even exist in our repo :-\

lib/Varien/Image/Adapter/Imagick.php Outdated Show resolved Hide resolved
Co-authored-by: Colin Mollenhour <colin@mollenhour.com>
@fballiano fballiano marked this pull request as draft September 18, 2023 16:20
Copy link
Contributor

@fballiano fballiano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • needs to implement gif, jpg, png, xbm, bmp, webp
  • needs to implement the watermark method

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: lib/Varien Relates to lib/Varien Component: lib/* Relates to lib/*
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants